feat: top of block auction and block-sdk integration #8058
Conversation
WalkthroughThis update integrates the block SDK and auction modules into the Osmosis application, enhancing transaction handling through new ante handlers and lane configurations. It also introduces auction functionality and adjusts the app structure to accommodate these new features, ensuring efficient transaction categorization and handling. Changes
Assessment against linked issues
Poem
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (5)
Files skipped from review due to trivial changes (1)
Files skipped from review as they are similar to previous changes (2)
Additional comments not posted (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
Important Notice This PR modifies an in-repo Go module. It is one of:
The dependent Go modules, especially the root one, will have to be Please follow the instructions below:
Please let us know if you need any help. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Actionable comments outside the diff hunks (9)
CHANGELOG.md (9)
Line range hint
322-322: Correct the heading punctuation.-## [v4.0.0] +## v4.0.0
Line range hint
585-585: Convert the bare URL to a markdown link for better readability.- https://github.com/osmosis-labs/osmosis/blob/v5.x/networks/osmosis-1/upgrades/v5/guide.md + [here](https://github.com/osmosis-labs/osmosis/blob/v5.x/networks/osmosis-1/upgrades/v5/guide.md)
Line range hint
643-643: Convert the bare URL to a markdown link for better readability.- https://github.com/osmosis-labs/bech32-ibc/ + [Bech32IBC](https://github.com/osmosis-labs/bech32-ibc/)
Line range hint
983-983: Ensure heading levels increment by one level at a time.- #### [v6.4.0] + ### [v6.4.0]
Line range hint
995-995: Ensure heading levels increment by one level at a time.- #### [v6.3.1] + ### [v6.3.1]
Line range hint
1004-1004: Ensure heading levels increment by one level at a time.- #### [v6.3.0] + ### [v6.3.0]
Line range hint
1009-1009: Ensure heading levels increment by one level at a time.- #### [v6.2.0] + ### [v6.2.0]
Line range hint
1061-1061: Ensure heading levels increment by one level at a time.- #### [v6.1.0] + ### [v6.1.0]
Line range hint
1224-1224: Ensure there are no empty links in the document.- [here]() + [here](https://example.com) # Please replace 'https://example.com' with the actual URL.
PaddyMc
force-pushed
the
feat/top-of-block-auction
branch
from
492f850 to
e37caae
Compare
PaddyMc
force-pushed
the
feat/top-of-block-auction
branch
from
e37caae to
d35fe80
Compare
| @@ -1,4 +1,6 @@ | |||
| go 1.21 | |||
| go 1.21.4 | |||
There was a problem hiding this comment.
Needed to upgrade to this version because of the block-sdk see:
https://github.com/skip-mev/block-sdk/blob/v1.4.0/go.mod#L3-L5
| appKeepers.BankKeeper, | ||
| appKeepers.DistrKeeper, | ||
| appKeepers.StakingKeeper, | ||
| authtypes.NewModuleAddress(govtypes.ModuleName).String(), |
There was a problem hiding this comment.
Rewards go to the gov module address 👀
There was a problem hiding this comment.
My comment incorrect, it uses a function to send rewards to validators
https://github.com/skip-mev/block-sdk/blob/main/x/auction/rewards/proposer_provider.go#L29-L41
There was a problem hiding this comment.
Actionable comments posted: 1
Actionable comments outside the diff hunks (3)
CHANGELOG.md (3)
Line range hint
989-1015: Correct heading levels to increment by one level at a time.-#### Golang API breaks +### Golang API breaks -#### Bug Fixes +### Bug Fixes -#### Features +### Features -#### SDK fork updates +### SDK fork updates -#### IAVL fork updates +### IAVL fork updates
Line range hint
591-649: Provide descriptive text for bare URLs to improve readability and accessibility.-https://github.com/osmosis-labs/osmosis/releases/tag/v1.0.0-rc0 +[Initial Release](https://github.com/osmosis-labs/osmosis/releases/tag/v1.0.0-rc0) -https://github.com/osmosis-labs/osmosis/releases/tag/v1.0.1 +[This release fixes a bug in `osmosisd version` always displaying 0.0.1.](https://github.com/osmosis-labs/osmosis/releases/tag/v1.0.1)
Line range hint
1230-1230: Remove or provide a valid URL for the empty link.-[null](#) +[Provide a valid link or remove this entry.](#)
There was a problem hiding this comment.
Actionable comments posted: 0
Actionable comments outside the diff hunks (8)
CHANGELOG.md (8)
Line range hint
325-325: Correct the heading punctuation.-## v6.4.0 +## v6.4.0.
Line range hint
588-588: Convert the bare URL to a markdown link for better readability.-https://github.com/osmosis-labs/bech32-ibc/ +[Bech32IBC](https://github.com/osmosis-labs/bech32-ibc/)
Line range hint
646-646: Convert the bare URL to a markdown link for better readability.-https://github.com/cosmos/cosmos-sdk/releases/tag/v0.43.0 +[SDK v0.43.0 Release Notes](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.43.0)
Line range hint
986-986: Ensure heading levels increment by one level at a time.-#### Golang API breaks +### Golang API breaks
Line range hint
998-998: Ensure heading levels increment by one level at a time.-#### Bug Fixes +### Bug Fixes
Line range hint
1007-1007: Ensure heading levels increment by one level at a time.-#### Features +### Features
Line range hint
1012-1012: Ensure heading levels increment by one level at a time.-#### SDK fork updates +### SDK fork updates
Line range hint
1064-1064: Ensure heading levels increment by one level at a time.-#### IAVL fork updates +### IAVL fork updates
| MaxBundleSize: 5, | ||
| ReserveFee: sdk.NewCoin(AuctionUSDCDenom, sdk.NewInt(1000000)), | ||
| MinBidIncrement: sdk.NewCoin(AuctionUSDCDenom, sdk.NewInt(1000000)), | ||
| EscrowAccountAddress: auctiontypes.DefaultEscrowAccountAddress, |
There was a problem hiding this comment.
escrow account is the module account for the auction module
| // we use a no-op ProcessProposal, this way, we accept all proposals in avoidance | ||
| // of liveness failures due to Prepare / Process inconsistency. In other words, | ||
| // this ProcessProposal always returns ACCEPT. | ||
| app.SetProcessProposal(baseapp.NoOpProcessProposal()) |
There was a problem hiding this comment.
just to be sure, we're no-op'ing process proposal along with block sdk integration correct? If so, what's the reason this is ok? 👀
There was a problem hiding this comment.
Do any other chains utilize a noOpProcessPropisal? Just curious from a testing perspective what the overhead of this will need to be if we are the first to do so.
There was a problem hiding this comment.
Neutron do:
https://github.com/neutron-org/neutron/blob/main/app/app.go#L1039-L1050
Also notes on why this was chosen are in notion:
https://www.notion.so/osmosiszone/Block-SDK-Mempool-Documentation-Roman-pt-2-d9af058c90ab4aaa8b48b630857b1916?pvs=4#82ae4f7525644a3aaaf181141c01aa22
😄
There was a problem hiding this comment.
Ah also from David:
https://github.com/skip-mev/osmosis/pull/2/files#r1548522449
"Note: this is set to no-op because the upstream priority nonce mempool that the block SDK utilizes under the hood does not natively support multi-fee denoms. As such the ordering may not be deterministic between proposal creation and verificaiton. This would require an integration with osmosis prices / slinky to turn on the ProcessProposalHandler."
| MaxBlockSpace: defaultLaneBlockspacePercentage, | ||
| SignerExtractor: signerAdapter, | ||
| MaxTxs: maxTxPerDefaultLane, | ||
| } |
There was a problem hiding this comment.
So total 3500 txs per block! Wondering if there were reasons these numbers of txs were chose or were arbitrary numbers
There was a problem hiding this comment.
They are just arbitrary numbers that the SKIP team suggested
There was a problem hiding this comment.
Its really confusing to me why there is a percentage and a maxTxs number. Our framing should be in terms of block space categories relative to one another rather than a number of transactions.
There was a problem hiding this comment.
The maxTx is a mempool setting, no? These field names from the block SDK can use some improvement imo
There was a problem hiding this comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Actionable comments outside the diff hunks (4)
CHANGELOG.md (4)
Line range hint
326-326: Correct the heading punctuation.-## v6.4.0. +## v6.4.0
Line range hint
589-589: Replace the bare URL with a markdown link.-https://github.com/osmosis-labs/cosmos-sdk/pull/136 +[sdk-#136](https://github.com/osmosis-labs/cosmos-sdk/pull/136)
Line range hint
647-647: Replace the bare URL with a markdown link.-https://github.com/osmosis-labs/iavl/pull/19 +[iavl-19](https://github.com/osmosis-labs/iavl/pull/19)
Line range hint
1228-1228: Ensure that the link is not empty.-[*](https://github.com/osmosis-labs/osmosis/pull/1228) +[*Description or title of the pull request*](https://github.com/osmosis-labs/osmosis/pull/1228)
| // we use a no-op ProcessProposal, this way, we accept all proposals in avoidance | ||
| // of liveness failures due to Prepare / Process inconsistency. In other words, | ||
| // this ProcessProposal always returns ACCEPT. | ||
| app.SetProcessProposal(baseapp.NoOpProcessProposal()) |
There was a problem hiding this comment.
Do any other chains utilize a noOpProcessPropisal? Just curious from a testing perspective what the overhead of this will need to be if we are the first to do so.
| MaxBlockSpace: defaultLaneBlockspacePercentage, | ||
| SignerExtractor: signerAdapter, | ||
| MaxTxs: maxTxPerDefaultLane, | ||
| } |
There was a problem hiding this comment.
Its really confusing to me why there is a percentage and a maxTxs number. Our framing should be in terms of block space categories relative to one another rather than a number of transactions.
| MinBidIncrement: sdk.NewCoin(AuctionUSDCDenom, sdk.NewInt(1000000)), | ||
| EscrowAccountAddress: auctiontypes.DefaultEscrowAccountAddress, | ||
| FrontRunningProtection: true, | ||
| ProposerFee: osmomath.MustNewDecFromStr("0.05"), |
There was a problem hiding this comment.
How do the funds actually go to the proposer? Does it get sent to the validator's stakers? To the validators personal operator address?
There was a problem hiding this comment.
In the auction ante handler => https://github.com/skip-mev/block-sdk/blob/v1.4.2/x/auction/ante/ante.go#L73-L75
The ValidateBidInfo on the auctionkeeper which calls ValidateAuctionBid:
https://github.com/skip-mev/block-sdk/blob/v1.4.2/x/auction/keeper/auction.go#L24-L26
ValidateAuctionBid calls ExtractBid here:
https://github.com/skip-mev/block-sdk/blob/v1.4.2/x/auction/keeper/auction.go#L91
Which sends funds to the escrow account, sans the proposer fee:
https://github.com/skip-mev/block-sdk/blob/v1.4.2/x/auction/keeper/auction.go#L91
Which will send the percent of funds to the rewardsAddress => https://github.com/skip-mev/block-sdk/blob/v1.4.2/x/auction/keeper/auction.go#L110
And the rewards address is defined here in a function:
https://github.com/skip-mev/block-sdk/blob/main/x/auction/rewards/proposer_provider.go#L29-L41
There was a problem hiding this comment.
So its the operator address, gotcha
| go 1.21.4 | ||
|
|
||
| toolchain go1.22.0 |
There was a problem hiding this comment.
I think we should do this bump in it's own PR if we decide to do it, and add a CHANGELOG specifically for this
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (9)
CHANGELOG.md (9)
Line range hint
988-988: Correct the heading level for better document structure.- ## Minor improvements & Bug Fixes + ### Minor improvements & Bug Fixes
Line range hint
1000-1000: Correct the heading level for better document structure.- ## Minor improvements & Bug Fixes + ### Minor improvements & Bug Fixes
Line range hint
1009-1009: Correct the heading level for better document structure.- ## Features + ### Features
Line range hint
1014-1014: Correct the heading level for better document structure.- ## Minor improvements & Bug Fixes + ### Minor improvements & Bug Fixes
Line range hint
1066-1066: Correct the heading level for better document structure.- ## Minor improvements & Bug Fixes + ### Minor improvements & Bug Fixes
Line range hint
327-327: Remove the trailing punctuation in the heading for consistency.- ## [v6.1.0] + ## [v6.1.0]
Line range hint
590-590: Convert the bare URL to a markdown link for better readability.- See: [SDK v0.43.0 Release Notes](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.43.0) For more details + See: [SDK v0.43.0 Release Notes](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.43.0) For more details
Line range hint
648-648: Convert the bare URL to a markdown link for better readability.- Implements Osmosis [prop32](https://www.mintscan.io/osmosis/proposals/32) -- clawing back the final 20% of unclaimed osmo and ion airdrop. + Implements Osmosis [prop32](https://www.mintscan.io/osmosis/proposals/32) -- clawing back the final 20% of unclaimed osmo and ion airdrop.
Line range hint
1229-1229: Ensure that there are no empty links in the document.- * [Patch](https://github.com/osmosis-labs/osmosis/commit/907001b08686ed980e0afa3d97a9c5e2f095b79f#diff-a172cedcae47474b615c54d510a5d84a8dea3032e958587430b413538be3f333) - Revert back to passing in the correct staking keeper into the IBC keeper constructor. + * [Patch](https://github.com/osmosis-labs/osmosis/commit/907001b08686ed980e0afa3d97a9c5e2f095b79f#diff-a172cedcae47474b615c54d510a5d84a8dea3032e958587430b413538be3f333) - Revert back to passing in the correct staking keeper into the IBC keeper constructor.
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
app/ante_no_seq.go (1)
33-38: The definition ofBlockSDKAnteHandlerParamsstruct is clear and well-structured. However, consider adding comments to describe each field, especiallymevLaneandauctionKeeper, to improve code readability and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (9)
CHANGELOG.md (9)
Line range hint
1002-1002: Heading levels should only increment by one level at a time.
Line range hint
1014-1014: Heading levels should only increment by one level at a time.
Line range hint
1023-1023: Heading levels should only increment by one level at a time.
Line range hint
1028-1028: Heading levels should only increment by one level at a time.
Line range hint
1080-1080: Heading levels should only increment by one level at a time.
Line range hint
341-341: Avoid using trailing punctuation in headings.
Line range hint
604-604: Avoid using bare URLs. Provide descriptive text with hyperlinks.
Line range hint
662-662: Avoid using bare URLs. Provide descriptive text with hyperlinks.
Line range hint
1243-1243: Ensure that hyperlinks are not empty and provide a valid destination.
There was a problem hiding this comment.
Actionable comments posted: 2
Out of diff range and nitpick comments (9)
CHANGELOG.md (9)
Line range hint
1006-1006: Adjust heading levels to increment by one level at a time.
Line range hint
1018-1018: Adjust heading levels to increment by one level at a time.
Line range hint
1027-1027: Adjust heading levels to increment by one level at a time.
Line range hint
1032-1032: Adjust heading levels to increment by one level at a time.
Line range hint
1084-1084: Adjust heading levels to increment by one level at a time.
Line range hint
345-345: Remove trailing punctuation from heading.
Line range hint
608-608: Replace bare URL with a descriptive link text.
Line range hint
666-666: Replace bare URL with a descriptive link text.
Line range hint
1247-1247: Ensure links are not empty and are correctly formatted.
| @@ -44,6 +44,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| ### State Breaking | |||
|
|
|||
| * [#7935](https://github.com/osmosis-labs/osmosis/pull/7935) Add block sdk and top of block auction from skip-mev. | |||
There was a problem hiding this comment.
Adjust heading levels to increment by one level at a time.
| // ante.NewIncrementSequenceDecorator(accountKeeper), | ||
| // auction module antehandler | ||
| auctionante.NewAuctionDecorator( | ||
| blockSDKParams.auctionKeeper, | ||
| blockSDKParams.txConfig.TxEncoder(), | ||
| blockSDKParams.mevLane, | ||
| ), |
There was a problem hiding this comment.
The auction module antehandler is duplicated in the authenticator verification flow. Consider if this duplication is necessary or if it can be streamlined to improve performance and maintainability.
Closes: #8062
In This PR
/app/upgrades/v25MinReserveFee- The minimum amount you need to outbid the highest bid (think of as a spam prevention)AuctionReserveFee- The minimum amount an address can bid (as ansdk.Coin)EscrowAddress- The address to which the auction revenue less proposer fee should goProposerFee- What % of auction revenue should go to a proposer (currently 20%)adds to this PR => skip-mev#2
🤜 🤛 From Skip!
In This PR
LaneMempoolwhich will be used as the app'sMempoolx/auctionmodule, and the correspondingAnteHandlerfor the auction moduleante.IgnoreDecoratorwith thefreeLane, and theDeductFeesante-handler: this will skip theDeductFeesante-handler for any txs that originate from thefreeLaneTesting
ante_no_seqante-handlers to ignore signature verification, while incrementing sequence #s, I leave a comment in the section describing why.Let's Discuss
Auctionmodule's parameters are here, take a look at these or we can go ahead w/ the defaults, some things to considerdistributiontypes.WithdrawDelegatorRewardstx, the process of adding more txs to this lane is simple, we just need to identify which types we want!Summary by CodeRabbit
New Features
Refactor
Chores
Documentation
Tests